doc: child_process: clarify `subprocess.pid` can be `undefined` when `ENOENT` by dr-js · Pull Request #37014 · nodejs/node

dr-js · 2021-01-21T03:18:14Z

From the code nodejs@8 and up should behave the same: https://github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290-L316

And a short test snippet:

const { spawn } = require('child_process')

const subProcess = spawn('non-exist-command')
subProcess.on('error', (error) => console.warn('mute Unhandled "error" event:', error))
console.log('- pid:', subProcess.pid)
process.nextTick(() => console.log('- after error emit'))
console.log('== end of test ==')

aduh95 · 2021-01-21T16:10:23Z

Do you want to update the test to make sure there's no regression in the future?

node/test/parallel/test-child-process-spawn-error.js

Lines 29 to 42 in 3df5afb

    
           const enoentPath = 'foo123'; 
        
           const spawnargs = ['bar']; 
        
           assert.strictEqual(fs.existsSync(enoentPath), false); 
        
           const enoentChild = spawn(enoentPath, spawnargs); 
        
           // Verify that stdio is setup if the error is not EMFILE or ENFILE. 
        
           assert.notStrictEqual(enoentChild.stdin, undefined); 
        
           assert.notStrictEqual(enoentChild.stdout, undefined); 
        
           assert.notStrictEqual(enoentChild.stderr, undefined); 
        
           assert(Array.isArray(enoentChild.stdio)); 
        
           assert.strictEqual(enoentChild.stdio[0], enoentChild.stdin); 
        
           assert.strictEqual(enoentChild.stdio[1], enoentChild.stdout); 
        
           assert.strictEqual(enoentChild.stdio[2], enoentChild.stderr);

dr-js · 2021-01-22T00:13:51Z

Sure, I'll try update the test.

dr-js · 2021-01-22T02:12:35Z

@aduh95 Upon adding the test, I found the pid value is a bit more complex than I thought:

an async spawn with invalid cwd will get pid undefined (expected)
an async spawn with invalid commad and shell: false will get pid undefined (expected)
an async spawn with invalid commad and shell: true will get pid of the shell (expected)
a sync spawn seems always return pid (strange)

Should I add more detailed behavior description to the doc and update test for async and sync spawn,
or keep it undocumented behavior as it's not so straightforward for now?

The test code for this:

[test.js]

[
  () => testSpawn('with invalid command', [ 'non-exist-command' ]),
  () => testSpawn('with invalid command and `shell: true`', [ 'non-exist-command', { shell: true } ]),
  () => testSpawn('with invalid cwd', [ 'node', { cwd: './non/exist/cwd/path/' } ]),
  () => testSpawn('invalid cwd and `shell: true`', [ 'node', { cwd: './non/exist/cwd/path/', shell: true } ]),

  () => testSpawnSync('with invalid command', [ 'non-exist-command' ]),
  () => testSpawnSync('with invalid command and `shell: true`', [ 'non-exist-command', { shell: true } ]),
  () => testSpawnSync('with invalid cwd', [ 'node', { cwd: './non/exist/cwd/path/' } ]),
  () => testSpawnSync('invalid cwd and `shell: true`', [ 'node', { cwd: './non/exist/cwd/path/', shell: true } ])
].forEach((testFunc, index) => setTimeout(testFunc, index * 200))

const formatError = (error) => error && error.message
// const formatError = (error) => error && error.stack // for more detail

const testSpawn = (title = '', spawnArgs = '') => {
  console.log(`\n== test spawn ${title} ==`)
  const { spawn } = require('child_process')
  const subProcess = spawn(...spawnArgs)
  subProcess.on('error', (error) => console.warn('- "error" event:', formatError(error))) // mute Unhandled "error" event or the test will exit
  console.log('- pid:', subProcess.pid, 'exitCode:', subProcess.exitCode)
  process.nextTick(() => console.log('- nextTick pid:', subProcess.pid, 'exitCode:', subProcess.exitCode))
  setTimeout(() => console.log('- after 100ms pid:', subProcess.pid, 'exitCode:', subProcess.exitCode), 100)
}

const testSpawnSync = (title = '', spawnArgs = '') => {
  console.log(`\n== test spawnSync ${title} ==`)
  const { spawnSync } = require('child_process')
  const outcome = spawnSync(...spawnArgs) // NOTE: not a `subProcess`
  console.log('- pid:', outcome.pid, 'status', outcome.status)
  console.log('- error', formatError(outcome.error))
}

And the output:

[v15.6.0 linux x64]

v15.6.0 linux x64

== test spawn with invalid command ==
- pid: undefined exitCode: null
- "error" event: spawn non-exist-command ENOENT
- nextTick pid: undefined exitCode: -2
- after 100ms pid: undefined exitCode: -2

== test spawn with invalid command and `shell: true` ==
- pid: 30493 exitCode: null
- nextTick pid: 30493 exitCode: null
- after 100ms pid: 30493 exitCode: 127

== test spawn with invalid cwd ==
- pid: undefined exitCode: null
- "error" event: spawn node ENOENT
- nextTick pid: undefined exitCode: -2
- after 100ms pid: undefined exitCode: -2

== test spawn invalid cwd and `shell: true` ==
- pid: undefined exitCode: null
- "error" event: spawn /bin/sh ENOENT
- nextTick pid: undefined exitCode: -2
- after 100ms pid: undefined exitCode: -2

== test spawnSync with invalid command ==
- pid: 30496 status null
- error spawnSync non-exist-command ENOENT

== test spawnSync with invalid command and `shell: true` ==
- pid: 30497 status 127
- error undefined

== test spawnSync with invalid cwd ==
- pid: 30498 status null
- error spawnSync node ENOENT

== test spawnSync invalid cwd and `shell: true` ==
- pid: 30499 status null
- error spawnSync /bin/sh ENOENT

[v15.6.0 win32 x64]

v15.6.0 win32 x64

== test spawn with invalid command ==
- pid: undefined exitCode: null
- "error" event: spawn non-exist-command ENOENT
- nextTick pid: undefined exitCode: -4058
- after 100ms pid: undefined exitCode: -4058

== test spawn with invalid command and `shell: true` ==
- pid: 21860 exitCode: null
- nextTick pid: 21860 exitCode: null
- after 100ms pid: 21860 exitCode: 1

== test spawn with invalid cwd ==
- pid: undefined exitCode: null
- "error" event: spawn node ENOENT
- nextTick pid: undefined exitCode: -4058
- after 100ms pid: undefined exitCode: -4058

== test spawn invalid cwd and `shell: true` ==
- pid: undefined exitCode: null
- "error" event: spawn C:\Windows\system32\cmd.exe ENOENT
- nextTick pid: undefined exitCode: -4058
- after 100ms pid: undefined exitCode: -4058

== test spawnSync with invalid command ==
- pid: 0 status null
- error spawnSync non-exist-command ENOENT

== test spawnSync with invalid command and `shell: true` ==
- pid: 8864 status 1
- error undefined

== test spawnSync with invalid cwd ==
- pid: 0 status null
- error spawnSync node ENOENT

== test spawnSync invalid cwd and `shell: true` ==
- pid: 0 status null
- error spawnSync C:\Windows\system32\cmd.exe ENOENT

aduh95 · 2021-01-22T09:30:48Z

Hum that's weird indeed, that might be a bug with spawnSync implementation. The "good news" is that the part of the docs you are modifying only covers the async version, the "bad news" is we now have to investigate for the pid in sync calls. Do you want to open an issue for the PID, and we can add a test in test/known_issues?

dr-js · 2021-01-22T11:48:58Z

@aduh95 Sure, so in this PR I'll add docs & test checks for subProcess.pid, as it's only from the async version of spawn.

And I'll open another Issue with above test to discuss sync spawn pid behavior, and maybe a PR to add test to test/known_issues.

doc/api/child_process.md

Trott · 2021-01-31T14:58:17Z

LGTM with doc updates. @nodejs/documentation @nodejs/child_process

nodejs-github-bot · 2021-02-19T11:48:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/36242/

From the code `nodejs@8` and up should behave the same: github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290 And a short test snippet: ```js const { spawn } = require('child_process') const subProcess = spawn('non-exist-command') subProcess.on('error', (error) => console.warn('mute Unhandled "error" event:', error)) console.log('- pid:', subProcess.pid) process.nextTick(() => console.log('- after error emit')) console.log('== end of test ==') ``` Note: the sync spawn result `pid` currently do not follow this pattern. Co-authored-by: Rich Trott <rtrott@gmail.com> PR-URL: nodejs#37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Note: this only add checks for async spawn, as the sync spawn do not return a `subProcess`. PR-URL: nodejs#37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 · 2021-02-22T11:17:23Z

Landed in 574590d...1e0cfa3

From the code `nodejs@8` and up should behave the same: github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290 And a short test snippet: ```js const { spawn } = require('child_process') const subProcess = spawn('non-exist-command') subProcess.on('error', (error) => console.warn('mute Unhandled "error" event:', error)) console.log('- pid:', subProcess.pid) process.nextTick(() => console.log('- after error emit')) console.log('== end of test ==') ``` Note: the sync spawn result `pid` currently do not follow this pattern. Co-authored-by: Rich Trott <rtrott@gmail.com> PR-URL: #37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Note: this only add checks for async spawn, as the sync spawn do not return a `subProcess`. PR-URL: #37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

From the code `nodejs@8` and up should behave the same: github.com/nodejs/node/blame/v8.17.0/lib/internal/child_process.js#L290 And a short test snippet: ```js const { spawn } = require('child_process') const subProcess = spawn('non-exist-command') subProcess.on('error', (error) => console.warn('mute Unhandled "error" event:', error)) console.log('- pid:', subProcess.pid) process.nextTick(() => console.log('- after error emit')) console.log('== end of test ==') ``` Note: the sync spawn result `pid` currently do not follow this pattern. Co-authored-by: Rich Trott <rtrott@gmail.com> PR-URL: #37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Note: this only add checks for async spawn, as the sync spawn do not return a `subProcess`. PR-URL: #37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jan 21, 2021

Trott reviewed Jan 24, 2021

View reviewed changes

doc/api/child_process.md Outdated Show resolved Hide resolved

dr-js force-pushed the doc-subprocess-pid branch from 9933250 to fd5748b Compare January 25, 2021 03:55

dr-js mentioned this pull request Jan 25, 2021

child_process: sync spawn should not return pid number of a non-exist-command #37057

Open

Trott reviewed Jan 31, 2021

View reviewed changes

doc/api/child_process.md Outdated Show resolved Hide resolved

Trott reviewed Jan 31, 2021

View reviewed changes

doc/api/child_process.md Outdated Show resolved Hide resolved

Trott reviewed Jan 31, 2021

View reviewed changes

doc/api/child_process.md Outdated Show resolved Hide resolved

dr-js force-pushed the doc-subprocess-pid branch from ec48ec2 to 61f86f3 Compare February 1, 2021 02:26

aduh95 approved these changes Feb 19, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 19, 2021

aymen94 approved these changes Feb 22, 2021

View reviewed changes

dr-js added 2 commits February 22, 2021 12:15

test,child_process: add check for subProcess.pid

1e0cfa3

Note: this only add checks for async spawn, as the sync spawn do not return a `subProcess`. PR-URL: nodejs#37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

aduh95 force-pushed the doc-subprocess-pid branch from 61f86f3 to 1e0cfa3 Compare February 22, 2021 11:16

aduh95 merged commit 1e0cfa3 into nodejs:master Feb 22, 2021

targos mentioned this pull request Mar 2, 2021

v15.11.0 release proposal #37569

Merged

dr-js deleted the doc-subprocess-pid branch March 4, 2021 13:18

targos pushed a commit that referenced this pull request May 1, 2021

test,child_process: add check for subProcess.pid

51ef745

Note: this only add checks for async spawn, as the sync spawn do not return a `subProcess`. PR-URL: #37014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

doc: child_process: clarify `subprocess.pid` can be `undefined` when `ENOENT`#37014

doc: child_process: clarify `subprocess.pid` can be `undefined` when `ENOENT`#37014
aduh95 merged 2 commits intonodejs:masterfrom
dr-js:doc-subprocess-pid

dr-js commented Jan 21, 2021

Uh oh!

aduh95 commented Jan 21, 2021

Uh oh!

dr-js commented Jan 22, 2021

Uh oh!

dr-js commented Jan 22, 2021 •

edited

Loading

Uh oh!

aduh95 commented Jan 22, 2021

Uh oh!

dr-js commented Jan 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Jan 31, 2021

Uh oh!

nodejs-github-bot commented Feb 19, 2021

Uh oh!

aduh95 commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

dr-js commented Jan 21, 2021

Uh oh!

aduh95 commented Jan 21, 2021

Uh oh!

dr-js commented Jan 22, 2021

Uh oh!

dr-js commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Jan 22, 2021

Uh oh!

dr-js commented Jan 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Jan 31, 2021

Uh oh!

nodejs-github-bot commented Feb 19, 2021

Uh oh!

aduh95 commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dr-js commented Jan 22, 2021 •

edited

Loading